Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expand foreign key references in row view as well #2031

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Expand foreign key references in row view as well #2031

wants to merge 1 commit into from

Conversation

tmcl-it
Copy link

@tmcl-it tmcl-it commented Mar 1, 2023 •
edited by github-actions bot
Loading

Unlike the table view, the single row view does not resolve foreign key references into labels. This patch extracts the foreign key reference expansion code from TableView.data() into a standalone function that is then called by both TableView.data() and RowView.data().

📚 Documentation preview 📚: https://datasette--2031.org.readthedocs.build/en/2031/

@simonw
Copy link
Owner

simonw commented Mar 6, 2023 •
edited
Loading

This is a very neat fix, for something I've been wanting for a while.

Add a unit test for the row HTML page - I suggest against this page: https://latest.datasette.io/fixtures/foreign_key_references/1 - and I'll land this PR.

You can model it on this test here:

datasette/tests/test_table_html.py

Lines 609 to 632 in a53b893

@pytest.mark.asyncio
async def test_table_html_foreign_key_links(ds_client):
response = await ds_client.get("/fixtures/foreign_key_references")
assert response.status_code == 200
table = Soup(response.text, "html.parser").find("table")
actual = [[str(td) for td in tr.select("td")] for tr in table.select("tbody tr")]
assert actual == [
[
'<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/1">1</a></td>',
'<td class="col-foreign_key_with_label type-str"><a href="/fixtures/simple_primary_key/1">hello</a>\xa0<em>1</em></td>',
'<td class="col-foreign_key_with_blank_label type-str"><a href="/fixtures/simple_primary_key/3">-</a>\xa0<em>3</em></td>',
'<td class="col-foreign_key_with_no_label type-str"><a href="/fixtures/primary_key_multiple_columns/1">1</a></td>',
'<td class="col-foreign_key_compound_pk1 type-str">a</td>',
'<td class="col-foreign_key_compound_pk2 type-str">b</td>',
],
[
'<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/2">2</a></td>',
'<td class="col-foreign_key_with_label type-none">\xa0</td>',
'<td class="col-foreign_key_with_blank_label type-none">\xa0</td>',
'<td class="col-foreign_key_with_no_label type-none">\xa0</td>',
'<td class="col-foreign_key_compound_pk1 type-none">\xa0</td>',
'<td class="col-foreign_key_compound_pk2 type-none">\xa0</td>',
],
]

I think adding it to test_table_html.py is OK, even though it's technically for the row page and not the table page.

Thanks!

@codecov
Copy link

codecov bot commented Mar 6, 2023 •
edited
Loading

Codecov Report

Patch coverage: 97.72% and project coverage change: -0.36 ⚠️

Comparison is base (3feed1f) 92.46% compared to head (c8a2904) 92.11%.

❗ Current head c8a2904 differs from pull request most recent head ef25867. Consider uploading reports for the commit ef25867 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2031      +/-   ##
==========================================
- Coverage   92.46%   92.11%   -0.36%     
==========================================
  Files          38       38              
  Lines        5750     5556     -194     
==========================================
- Hits         5317     5118     -199     
- Misses        433      438       +5
Impacted Files Coverage Δ
datasette/views/table.py 92.57% <97.56%> (-3.35%) ⬇️
datasette/views/base.py 95.17% <100.00%> (+2.38%) ⬆️
datasette/views/row.py 87.93% <100.00%> (+0.10%) ⬆️

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tmcl-it tmcl-it force-pushed the patch-row-view-expand-labels branch 3 times, most recently from d39fa8b to 98c7206 Compare March 6, 2023 23:31
@tmcl-it
Copy link
Author

tmcl-it commented Mar 7, 2023 •
edited
Loading

I've implemented the test (thanks for pointing me in the right direction!).

At tmcl-it/datasette:0.64.1+row-view-expand-labels I also have a variant of this patch that applies to the 0.64.x branch. Please let me know if you'd be interested in merging that as well and I'll open another PR.

@simonw
Copy link
Owner

simonw commented Mar 9, 2023

I've implemented the test (thanks for pointing me in the right direction!).

At tmcl-it/datasette:0.64.1+row-view-expand-labels I also have a variant of this patch that applies to the 0.64.x branch. Please let me know if you'd be interested in merging that as well and I'll open another PR.

Sure, let's merge that one too - it can go out in the next 0.64.x series release (maybe even a 0.65).

@tmcl-it tmcl-it force-pushed the patch-row-view-expand-labels branch from 98c7206 to ef25867 Compare March 24, 2023 18:28
@tmcl-it
Copy link
Author

tmcl-it commented Mar 24, 2023

I've rebased my patch on the latest main. It should be ready to merge.

@tmcl-it tmcl-it mentioned this pull request Mar 24, 2023
Closed
@tmcl-it tmcl-it force-pushed the patch-row-view-expand-labels branch from ef25867 to d903e48 Compare November 30, 2024 16:39
@tmcl-it
Copy link
Author

tmcl-it commented Nov 30, 2024

Hi @simonw, I've again rebased my patch on the latest main. Could you merge it?

@tmcl-it tmcl-it mentioned this pull request Nov 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmcl-it @simonw